-
Couldn't load subscription status.
- Fork 116
fix: add explicit mapping for cancelled status #2291
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: add explicit mapping for cancelled status #2291
Conversation
Summary of ChangesHello @ab-ghosh, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves an issue where the GitLab provider did not correctly interpret the 'cancelled' status. It introduces a specific case to map 'cancelled' to 'canceled' for GitLab API compatibility and sets an appropriate status title. A corresponding unit test has been added to ensure the new handling works as expected. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
🔍 PR Lint Feedback
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request correctly adds support for the 'cancelled' status in the GitLab provider by mapping it to 'canceled' for API compatibility, and includes a unit test to verify the new logic. The implementation is sound. I have one suggestion to refactor the code slightly by combining similar cases, which would improve code clarity and maintainability.
|
can we fix canceled across the codebase ? |
|
I've reviewed the codebase and didn't find any other inconsistencies related to the use of “canceled” for GitLab. |
@chmouel what do you mean by fixing? removing? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel free to resolve the comment if it is incorrect, added the comment more for learning for myself.
|
Or I will go with failed CI as well because its just workflow yaml changes doesn't affect E2E anyway |
|
Bear in mind working on it, this PR make a good use case for user permission test and need to iterate with it |
@zakisk i mean this I thought we just standardize everywhere on the american wording (cancelled) instead of using canceled |
ah, okay makes sense! |
@ab-ghosh feel free to find and change the docs/log messages having "canceled" |
|
We need to figure out how to get the misspelling linter, or via golangci-lint, to flag for us some specific words used. I think it's possible if you can try to investigate that; that would be great, Abishek 🙏🏻 |
I think this might be helpful. |
@theakshaypant Yes, this should work, I was trying this. |
7133c5c to
922f701
Compare
.golangci.yml
Outdated
| - linters: | ||
| - misspell | ||
| path: pkg/provider/gitlab/gitlab\.go | ||
| text: '`canceled` is a misspelling of `cancelled`' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| text: '`canceled` is a misspelling of `cancelled`' | |
| text: 'found `canceled` it must be `cancelled`' |
you can't say that its misspelled as its just a other dialect of English some might choose to write 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe better:
use american english version of the word `cancelled` as `canceled` with one L
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zakisk this text field in the exclusion rule is not a custom message, it needs to exactly match the error message generated by the misspell linter itself. This is because golangci-lint uses it to pattern match against the linter's output when applying exclusions. In this case, I'm trying to exclude the files related to GitLab.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, yeah I didn't know that but in that can you can suppress the lint on CreateStatus func in gitlab.go using comment on it //nolint:misspell
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @zakisk! I will suppress the lint just for that function.
.golangci.yml
Outdated
| - linters: | ||
| - misspell | ||
| path: test/gitlab.*\.go | ||
| text: '`canceled` is a misspelling of `cancelled`' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
922f701 to
9787dcc
Compare
|
/test |
- Add case for 'cancelled' conclusion in GitLab provider CreateStatus - Map 'cancelled' to 'canceled' for GitLab API compatibility - Set descriptive title 'cancelled validating this commit' - Ensures proper status reporting when PipelineRuns are cancelled via PR close
Add misspell linter configuration to enforce consistent use of 'cancelled' (British spelling) across the codebase. Update all internal code, comments, and test assertions to use 'cancelled'. Exclude GitLab provider code and test files from this rule, as GitLab's API requires 'canceled' (American spelling) for compatibility. Changes: - Configure misspell linter in .golangci.yml - Fix spelling in pkg/cmd/tknpac and pkg/reconciler - Update test files to use 'cancelled' - Add exclusions for GitLab API-related code Signed-off-by: ab-ghosh <[email protected]>
9787dcc to
b040b54
Compare
📝 Description of the Change
👨🏻 Linked Jira
https://issues.redhat.com/browse/SRVKP-9050
🔗 Linked GitHub Issue
Fixes #
🚀 Type of Change
fix:)feat:)feat!:,fix!:)docs:)chore:)refactor:)enhance:)deps:)🧪 Testing Strategy
🤖 AI Assistance
If you have used AI assistance, please provide the following details:
Which LLM was used?
Extent of AI Assistance:
Important
If the majority of the code in this PR was generated by an AI, please add a
Co-authored-bytrailer to your commit message.For example:
Co-authored-by: Gemini [email protected]
Co-authored-by: ChatGPT [email protected]
Co-authored-by: Claude [email protected]
Co-authored-by: Cursor [email protected]
Co-authored-by: Copilot [email protected]
**💡You can use the script
./hack/add-llm-coauthor.shto automatically addthese co-author trailers to your commits.
✅ Submitter Checklist
fix:,feat:) matches the "Type of Change" I selected above.make testandmake lintlocally to check for and fix anyissues. For an efficient workflow, I have considered installing
pre-commit and running
pre-commit installtoautomate these checks.